[WIP]fix(eval): correct peak memory measurement by jac0626 · Pull Request #2522 · antgroup/vsag

jac0626 · 2026-07-23T11:00:52Z

Change Type

Linked Issue

Fixes: eval_performance Reports Incorrect Memory Usage (16777216.00 TB) #1109

What Changed

Replace the underflow-prone, point-in-time RSS calculation with a restartable background sampler that reads process RSS every 5 ms on Linux and macOS.
Preserve memory_peak(build/search) while adding raw baseline, absolute peak, delta, availability, sample-count, and error fields; clamp negative deltas to zero.
Measure only Index::Build() during build and the latency/QPS query pass during search; run KNN statistics outside monitored intervals.
Reuse one evaluation dataset in build,search mode and release the build index before creating/deserializing the search index.
Report index-owned memory through index_memory(B) in addition to process RSS and existing memory details.
Add regression tests, macOS eval-test coverage, and synchronized English/Chinese documentation.

Root Cause

The previous monitor subtracted two uint64_t RSS page counts without validating reads or ordering. If the final RSS was below the constructor-time baseline, subtraction wrapped near UINT64_MAX, producing 16777216.00 TB. Build also sampled only after completion, search used a later intrusive query pass, and build,search retained duplicate datasets and index objects.

Test Evidence

Test details:

cmake --build build --target unittests eval_performance --parallel 6
./build/tests/unittests '[eval]' --allow-running-no-tests
# All tests passed: 64 assertions in 11 test cases

clang-format-15 --dry-run --Werror <changed C++ files>
# Ubuntu clang-format version 15.0.7

python3 -c "import yaml; yaml.safe_load(open('.github/workflows/pr-ci.yml'))"
git diff --check

The eval tests also passed 50 consecutive repetitions. A temporary 20,000-vector HDF5 build,search end-to-end run completed successfully and produced sane byte/RSS fields without the oversized TB value.

Compatibility Impact

API/ABI compatibility: none; changes are confined to the eval tool, tests, CI, and documentation.
Behavior changes: memory output now represents sampled process-RSS peak delta for the measured phase. Existing human-readable memory_peak(build/search) keys remain available; additional diagnostics are emitted in JSON.

Performance and Concurrency Impact

Performance impact: no change to VSAG's Index::KnnSearch() implementation. When memory and latency are enabled together, one 5 ms RSS sampling thread runs concurrently and may add small benchmark noise; KNN statistics no longer contaminate latency/QPS measurements.
Concurrency/thread-safety impact: adds a phase-scoped sampler thread guarded by private synchronization; it does not take index or per-query locks.

Documentation Impact

Risk and Rollback

Risk level: medium; the change updates eval measurement boundaries and output diagnostics but does not modify the library API or index algorithms.
Rollback plan: revert commit af9b6b1e to restore the previous eval monitor and pass lifecycle.

Checklist

I have linked the relevant issue.
I have added/updated tests for the bug fix.
I have considered API compatibility impact.
I have updated docs for the behavior change.
My commit message follows project conventions.

Signed-off-by: jc543239 <jc543239@antgroup.com> Assisted-by: Codex:gpt-5

vsag-bot · 2026-07-23T11:00:56Z

/label status/blocked
/waiting-on author
/request-review @jiaweizone
/request-review @wxyucs
/request-review @inabao
/request-review @vsag-bot

vsag-bot · 2026-07-23T11:01:03Z

Automated pull request review failed.

Review effort: high (1328 changed lines across 18 files).

git exited with 128: Cloning into '/tmp/vsag-pull-request-reviews/f4b26dd0-a029-46c5-8cd1-b1edc7bd7277/source'...
fatal: unable to access 'https://github.com/antgroup/vsag.git/': Received HTTP code 503 from proxy after CONNECT

No GitHub review was submitted.

mergify · 2026-07-23T11:01:42Z

Merge Protections

🟢 All 3 merge protections satisfied — ready to merge.

Show 3 satisfied protections

🟢 Require kind label

label~=^kind/

🟢 Require version label

label~=^version/

🟢 Require linked issue for feature/bug PRs

body~=(?im)(?:^|[\s\-\*])(?:close[sd]?|fix(?:e[sd])?|resolve[sd]?)\s*:?\s+(?:#\d+|[\w.\-]+/[\w.\-]+#\d+|https?://github\.com/[\w.\-]+/[\w.\-]+/issues/\d+)

Copilot

Pull request overview

Fixes incorrect eval_performance peak memory reporting by replacing the underflow-prone RSS delta calculation with a phase-scoped background RSS sampler, while also tightening evaluation phase boundaries and enriching JSON diagnostics. This improves correctness of memory metrics (notably avoiding the multi-terabyte underflow artifact) without changing VSAG core APIs/ABI.

Changes:

Reworked MemoryPeakMonitor to sample process RSS in a restartable background thread, clamp negative deltas to zero, and emit detailed JSON diagnostics while preserving memory_peak(build/search).
Refactored search evaluation passes to isolate monitored query phases from KNN statistics collection via SearchPassRunner.
Added unit/regression coverage for the new monitor/pass scheduling and expanded CI/docs to reflect the updated metrics.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tools/eval/monitor/memory_peak_monitor.h	Redesigns the monitor API/state for a background RSS sampler and richer result fields.
tools/eval/monitor/memory_peak_monitor.cpp	Implements platform RSS readers (Linux/macOS), sampling loop, clamped deltas, and expanded JSON output.
tools/eval/monitor/memory_peak_monitor_test.cpp	Adds regression/unit tests for baseline/peak/delta behavior, restartability, and formatting.
tools/eval/CMakeLists.txt	Adds `search_pass_runner.cpp` to the eval tool build sources.
tools/eval/case/search_pass_runner.h	Introduces a helper to run monitored search passes with optional memory sampling coordination.
tools/eval/case/search_pass_runner.cpp	Implements pass scheduling to ensure memory sampling aligns with the intended query pass boundaries.
tools/eval/case/search_pass_runner_test.cpp	Adds tests validating pass order, memory-on/off behavior, and exception unwinding semantics.
tools/eval/case/search_eval_case.h	Extends constructor to optionally reuse an already-loaded dataset; adds helpers for pass separation.
tools/eval/case/search_eval_case.cpp	Uses `SearchPassRunner` to separate monitored query passes from statistics collection; adds `index_memory(B)` reporting.
tools/eval/case/eval_case.h	Extends factory/ctor to accept an optional shared dataset instance.
tools/eval/case/eval_case.cpp	Implements dataset reuse through `MakeInstance` and the base `EvalCase` constructor.
tools/eval/case/build_search_eval_case.h	Reuses one dataset across build+search and releases build index before creating/deserializing the search index.
tools/eval/case/build_eval_case.h	Extends build case constructor to accept an optional shared dataset.
tools/eval/case/build_eval_case.cpp	Tightens build measurement boundary around `Index::Build()` and adds `index_memory(B)` reporting.
tests/CMakeLists.txt	Adds eval monitor/pass sources to `unittests` (plus Threads/json linkage) to run new `[eval]` tests.
docs/docs/en/src/resources/eval.md	Documents the new RSS sampling semantics, phase boundaries, and additional JSON fields.
docs/docs/zh/src/resources/eval.md	Chinese documentation parity for the new memory metrics and boundaries.
.github/workflows/pr-ci.yml	Ensures `tools/**` triggers C++ CI and runs eval-tagged tests on macOS ASan builds.

+    const auto sample = read_memory();
+    std::lock_guard<std::mutex> lock(state_mutex_);
+    if (sample.available) {
+        absolute_peak_bytes_ = std::max(absolute_peak_bytes_, sample.bytes);
+        ++sample_count_;
+        return;
    }


wxyucs · 2026-07-24T06:41:20Z

/version 1.1

LHT129

Code review for PR #2522: [WIP]fix(eval): correct peak memory measurement

[suggestion] tools/eval/monitor/memory_peak_monitor.cpp:1949 — last_error_ not cleared on successful samples

In sample_memory(), last_error_ is never cleared when a sample succeeds. A transient failure leaves a stale error string even when subsequent samples succeed, making memory_peak_error(<phase>) misleading — it can report an old error despite a valid baseline and peak.

The failure count already captures transient issues; the error field should reflect the last sampling outcome. Suggested fix: add last_error_.clear(); in the success branch of sample_memory().

[suggestion] tools/eval/case/search_pass_runner.cpp:1316-1323 — MonitorPassGuard destruction order is implicit

In run_monitored_pass(), when memory_monitor is not null, memory_guard is declared after monitor_guard, so memory_guard destructs first (memory stops before the ordinary monitor). This is correct behavior, but it relies on C++ local variable declaration order which is easy to break during refactoring. Consider making the order explicit, e.g. by calling memory_guard.Stop() before the end of scope, or adding a comment noting the intentional ordering.

[suggestion] tools/eval/case/build_search_eval_case.h:87-89 — build_ null fallback creates ambiguity

In Run(), if build_ is null after std::move, the code falls back to EvalCase::MakeInstance(config_, "build", dataset_ptr_). This creates a new index via Factory::CreateIndex, not the original index passed to the constructor. Since the constructor always sets build_, this fallback path is unreachable in practice. Consider removing the dead code or adding a comment explaining when this path could be triggered.

[note] tools/eval/case/search_eval_case.cpp:553 — do_knn_filter_search() passes empty statistics callback

do_knn_filter_search() passes []() {} as the statistics_pass, meaning KNN result statistics are not collected in filter search mode. This differs from do_knn_search() which collects statistics after all monitor passes. This appears intentional (filter search does not collect recall statistics), but is worth confirming.

LHT129

[suggestion] Review of codex/fix-eval-memory-1109 (af9b6b1). This is a WIP PR so these are non-blocking suggestions.

1. `sample_memory()` does not clear `last_error_` on successful reads

In tools/eval/monitor/memory_peak_monitor.cpp, the sample_memory() method sets last_error_ when a sample fails but never clears it when a subsequent sample succeeds. This means once any transient failure occurs, memory_peak_error(<phase>) will report a stale error for the remainder of the measurement phase, even though the baseline and peak are valid.

Suggested fix: add last_error_.clear(); in the success branch of sample_memory().

2. `StartedMonitorGuard::StopNext()` naming is misleading

In tools/eval/case/build_eval_case.cpp, StopNext() always stops the last started monitor (reverse-start order), but the name suggests it stops the "next" one in forward order. The destructor already handles reverse-order unwind correctly. Consider renaming to StopLast() to document the contract, or removing StopNext() entirely and letting the destructor handle all stops.

LHT129 · 2026-08-10T14:20:41Z

+        absolute_peak_bytes_ = std::max(absolute_peak_bytes_, sample.bytes);
+        ++sample_count_;
+        return;
    }


[suggestion] last_error_ is never cleared after a successful sample in sample_memory(). If a transient failure occurs (e.g., a single /proc/self/statm read fails) and subsequent samples succeed, last_error_ retains the stale error string. This causes GetResult() to report a non-empty memory_peak_error even though the overall measurement succeeded.

The failure_count_ field already captures transient failures. Consider clearing last_error_ on successful samples so the error field reflects only the most recent outcome:

if (sample.available) { absolute_peak_bytes_ = std::max(absolute_peak_bytes_, sample.bytes); ++sample_count_; last_error_.clear(); // clear stale error on success return; }

LHT129 · 2026-08-10T14:20:55Z

+        build_ = std::make_shared<BuildEvalCase>(
+            dataset_path_, index_path_, std::move(index), config_, dataset_ptr_);
    }



[note] Run() now creates a new SearchEvalCase on every call via MakeInstance, whereas previously search_ was constructed once in the constructor. This means each Run() call will deserialize the index from disk again. While this is intentional (per the PR description, to release the build index before creating the search index), it is worth noting that Run() is no longer safe to call multiple times on the same BuildSearchEvalCase instance without re-incurring the full deserialization cost. If callers previously relied on Run() being repeatable without re-deserialization, this is a behavioral change.

LHT129 · 2026-08-10T14:22:38Z

+        absolute_peak_bytes_ = std::max(absolute_peak_bytes_, sample.bytes);
+        ++sample_count_;
+        return;
    }


[suggestion] last_error_ is never cleared after a successful sample. Once any transient failure occurs, last_error_ remains non-empty even if all subsequent samples succeed, causing memory_peak_error(<phase>) to report a stale error. The memory_peak_failed_sample_count already captures failure history, so last_error_ should reflect the most recent sampling outcome. In sample_memory(), add last_error_.clear(); in the success path before the return.

LHT129 · 2026-08-10T14:22:49Z

+    while (
+        not stop_condition_.wait_for(lock, sample_interval_, [this]() { return not running_; })) {
+        lock.unlock();
+        sample_memory();


[note] In sampling_loop(), the final lock.unlock(); sample_memory(); call runs sample_memory() without the mutex held. While sample_memory() internally acquires state_mutex_, the running_ flag is already false at this point, so the wait_for predicate is satisfied and the loop has exited. This final sample is intentional (to capture one last reading after the stop signal), but consider documenting this with a brief comment since the unlock-before-call pattern is unusual.

LHT129 · 2026-08-10T14:23:05Z

-        monitor->Record();
-        monitor->Stop();
+    for (auto monitor = monitors_.rbegin(); monitor != monitors_.rend(); ++monitor) {
+        (*monitor)->Record();


[note] MemoryPeakMonitor::Record() is now a no-op since sampling moved to the background thread. The call to monitor->Record() in do_build() (line 122) still executes but has no effect for MemoryPeakMonitor. This is harmless but may confuse future readers. Consider either removing the Record() call or adding a comment noting that MemoryPeakMonitor samples continuously in the background and ignores explicit Record() calls.

fix(eval): correct peak memory measurement

af9b6b1

Signed-off-by: jc543239 <jc543239@antgroup.com> Assisted-by: Codex:gpt-5

Copilot AI review requested due to automatic review settings July 23, 2026 11:00

pull-request-size Bot added the size/XXL label Jul 23, 2026

vsag-bot assigned LHT129 Jul 23, 2026

vsag-bot self-requested a review July 23, 2026 11:01

jac0626 added kind/bug Bug fixes, defects, or unexpected behavior 修复程序错误、缺陷或异常行为 version/1.0 labels Jul 23, 2026 — with ChatGPT Codex Connector

mergify Bot added module/docs module/tools module/testing labels Jul 23, 2026

Copilot started reviewing on behalf of jac0626 July 23, 2026 11:05 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

Comment thread tools/eval/monitor/memory_peak_monitor.cpp

Comment on lines +248 to 254

const auto sample = read_memory();

std::lock_guard<std::mutex> lock(state_mutex_);

if (sample.available) {

absolute_peak_bytes_ = std::max(absolute_peak_bytes_, sample.bytes);

++sample_count_;

return;

}

wxyucs marked this pull request as ready for review July 24, 2026 06:41

wxyucs requested review from LHT129, inabao, jiaweizone and wxyucs as code owners July 24, 2026 06:41

vsag-bot added version/1.1 and removed version/1.0 labels Jul 24, 2026

jac0626 marked this pull request as draft July 27, 2026 07:07

jac0626 changed the title ~~fix(eval): correct peak memory measurement~~ [WIP]fix(eval): correct peak memory measurement Jul 27, 2026

LHT129 reviewed Aug 7, 2026

View reviewed changes

LHT129 reviewed Aug 10, 2026

View reviewed changes

Uh oh!

Conversation

jac0626 commented Jul 23, 2026

Change Type

Linked Issue

What Changed

Root Cause

Test Evidence

Compatibility Impact

Performance and Concurrency Impact

Documentation Impact

Risk and Rollback

Checklist

Uh oh!

vsag-bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vsag-bot commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mergify Bot commented Jul 23, 2026

Merge Protections

🟢 Require kind label

🟢 Require version label

🟢 Require linked issue for feature/bug PRs

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

wxyucs commented Jul 24, 2026

Uh oh!

LHT129 left a comment

Choose a reason for hiding this comment

Uh oh!

LHT129 left a comment

Choose a reason for hiding this comment

1. sample_memory() does not clear last_error_ on successful reads

2. StartedMonitorGuard::StopNext() naming is misleading

Uh oh!

LHT129 Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

LHT129 Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

LHT129 Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

LHT129 Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

LHT129 Aug 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vsag-bot commented Jul 23, 2026 •

edited

Loading

vsag-bot commented Jul 23, 2026 •

edited

Loading

1. `sample_memory()` does not clear `last_error_` on successful reads

2. `StartedMonitorGuard::StopNext()` naming is misleading